USHIFT-6951: Add metrics-server as optional MicroShift component#6808
Open
copejon wants to merge 9 commits into
Open
USHIFT-6951: Add metrics-server as optional MicroShift component#6808copejon wants to merge 9 commits into
copejon wants to merge 9 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
WalkthroughThis PR adds metrics-server as an optional observability component to MicroShift. It includes Kubernetes manifests with RBAC and security hardening, TLS certificate provisioning, OpenTelemetry integration, RPM packaging, and build automation to generate per-architecture image overrides and release metadata during rebasing. ChangesMetrics-Server Component Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." ... [truncated 31032 characters] ... elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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
This was referenced Jun 5, 2026
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
a185390 to
2f00a69
Compare
![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: 9
🧹 Nitpick comments (1)
packaging/observability/microshift-observability.service (1)
11-11: ⚡ Quick winUse an argv array instead of a concatenated shell string
Building
ARGSas a string is brittle (word-splitting/path edge cases). Use a bash array and quoted expansion.Proposed change
-ExecStart=/bin/bash -c 'ARGS="--config=file:/etc/microshift/observability/opentelemetry-collector.yaml"; for f in /etc/microshift/observability/otelcol.d/*.yaml; do [ -f "$$f" ] && ARGS="$$ARGS --config=file:$$f"; done; exec /usr/bin/opentelemetry-collector $$ARGS' +ExecStart=/bin/bash -c 'args=(--config=file:/etc/microshift/observability/opentelemetry-collector.yaml); for f in /etc/microshift/observability/otelcol.d/*.yaml; do [ -f "$$f" ] && args+=(--config=file:$$f); done; exec /usr/bin/opentelemetry-collector "$${args[@]}"'🤖 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/observability/microshift-observability.service` at line 11, The ExecStart line builds a brittle space-joined ARGS string; change it to build a bash array and use quoted array expansion when executing the collector: initialize ARGS=() (or ARGS=()), append elements as ARGS+=(--config="file:/etc/microshift/observability/opentelemetry-collector.yaml") and inside the loop for f in /etc/microshift/observability/otelcol.d/*.yaml; do [ -f "$f" ] && ARGS+=(--config="file:$f"); done; finally exec /usr/bin/opentelemetry-collector "${ARGS[@]}" so that each --config is a separate argv element and you avoid word-splitting/path edge cases for the ExecStart/ARGS usage.
🤖 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/metrics-server/00-namespace.yaml`:
- Around line 7-9: The namespace-level PodSecurity labels
pod-security.kubernetes.io/enforce, pod-security.kubernetes.io/audit, and
pod-security.kubernetes.io/warn are set to "privileged" which is too permissive;
change their values to a safer profile (e.g., "baseline" or "restricted") unless
you have a documented exception, and ensure the three keys (enforce, audit,
warn) are updated consistently to the chosen profile so namespace-wide policies
default to the tightened posture.
In `@assets/optional/metrics-server/03-deployment.yaml`:
- Around line 67-70: The metrics-server container currently only specifies
resource requests; add a matching limits block under the same resources section
for the metrics-server container to enforce CPU and memory caps (e.g.,
limits.cpu: 100m and limits.memory: 100Mi). Update the Deployment's container
resources block (the metrics-server container in the manifest) to include both
requests and limits so it conforms to the "Resource limits (cpu, memory) on
every container" guideline.
- Around line 71-75: The container securityContext currently sets
allowPrivilegeEscalation, readOnlyRootFilesystem and runAsNonRoot but does not
drop Linux capabilities; update the Pod/Container spec by adding a capabilities
block under securityContext (e.g., in the same container spec that contains
allowPrivilegeEscalation/readOnlyRootFilesystem/runAsNonRoot) with
capabilities.drop set to ["ALL"] so all capabilities are removed by default and
then explicitly add back any minimal capabilities only if absolutely required
elsewhere; ensure the change is applied alongside the existing
terminationMessagePolicy and other securityContext fields.
In `@assets/optional/metrics-server/kustomization.yaml`:
- Around line 3-13: The kustomization.yaml resources list is missing the two
RPM-installed manifests; update the resources array (the same block that
currently lists 00-namespace.yaml, 03-deployment.yaml, 04-service.yaml, etc.) to
include network-policy-downstream.yaml and pod-disruption-budget.yaml so both
NetworkPolicy and PodDisruptionBudget are applied; add those two filenames to
the resources list in assets/optional/metrics-server/kustomization.yaml.
In `@pkg/cmd/metrics.go`:
- Around line 44-50: The polling closure passed to wait.PollUntilContextTimeout
currently treats any Get() error as "not ready" and keeps retrying; change the
logic in the closure used with wait.PollUntilContextTimeout so that after
calling clientset.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{}), if
err == nil return true,nil; if apierrors.IsNotFound(err) log and return
false,nil to keep retrying; for any other error return false, err (so the poll
stops and surfaces auth/TLS/transport failures). Ensure you import/use
apierrors.IsNotFound and update the closure around
clientset.CoreV1().Namespaces().Get accordingly.
- Around line 24-31: The current check uses
util.PathExists(metricsServerManifestPath) so provisioning proceeds whenever the
file exists even if the metrics component is not enabled; change the gating to
verify the component is enabled and that any configured kustomization paths are
present (e.g., read the configured kustomizationPaths or component flag used by
your installer and ensure they are enabled) before attempting cert provisioning.
Update the logic around util.PathExists(metricsServerManifestPath) to first
consult the metrics enablement/config (the same config that controls
installation) and validate each configured kustomization path exists, returning
early if the component is disabled or no configured paths are present.
In `@scripts/auto-rebase/assets_metrics.yaml`:
- Around line 34-37: Update the two YAML asset entries so they match the rest of
the PR naming convention: change the "file" values
"release-metrics-aarch64.json" and "release-metrics-x86_64.json" to
"release-metrics-server-aarch64.json" and "release-metrics-server-x86_64.json"
respectively in the assets_metrics.yaml file (the two "- file:" entries shown),
leaving the corresponding "ignore" values unchanged.
In `@scripts/auto-rebase/assets.yaml`:
- Around line 304-326: The asset list under the optional/metrics-server/ entry
is missing the two manifests that the packaging script expects; update the files
list for the optional/metrics-server/ dir to include entries for
network-policy-downstream.yaml and pod-disruption-budget.yaml so the asset set
matches packaging/rpm/microshift.spec; ensure the new entries use the same
indentation/format as the existing file: ... entries (e.g., add "- file:
network-policy-downstream.yaml" and "- file: pod-disruption-budget.yaml" under
the files: block for the optional/metrics-server/ dir).
In `@scripts/auto-rebase/rebase.sh`:
- Around line 1184-1187: The script currently skips unknown images when
release_tag is empty (checking release_tag, orig_image, component_dir) which
allows silent incomplete kustomization.*.yaml outputs; change the behavior to
fail-fast by replacing the continue path with a non-zero exit (e.g., >&2 echo
descriptive error including orig_image and component_dir, then exit 1) so the
rebase.sh run stops immediately on unmapped images and surfaces the problem to
CI.
---
Nitpick comments:
In `@packaging/observability/microshift-observability.service`:
- Line 11: The ExecStart line builds a brittle space-joined ARGS string; change
it to build a bash array and use quoted array expansion when executing the
collector: initialize ARGS=() (or ARGS=()), append elements as
ARGS+=(--config="file:/etc/microshift/observability/opentelemetry-collector.yaml")
and inside the loop for f in /etc/microshift/observability/otelcol.d/*.yaml; do
[ -f "$f" ] && ARGS+=(--config="file:$f"); done; finally exec
/usr/bin/opentelemetry-collector "${ARGS[@]}" so that each --config is a
separate argv element and you avoid word-splitting/path edge cases for the
ExecStart/ARGS usage.
🪄 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: 25baa466-07b1-4aa1-90d3-a9e96b03c015
📒 Files selected for processing (27)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/bin/common.sh
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
2f00a69 to
2552538
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/metrics-server/02-configmap-audit-profiles.yaml`:
- Around line 2-38: The embedded audit policy YAMLs are malformed because keys
and values are written with JSON-style quoted strings; update
metadata-profile.yaml, none-profile.yaml, request-profile.yaml and
requestresponse-profile.yaml inside the ConfigMap data to valid YAML by removing
the unnecessary quotes around keys (apiVersion, kind, metadata, name,
omitStages, rules, level) and unquoting list items (e.g., RequestReceived), and
ensure each rule is a proper mapping (e.g., - level: Metadata) with correct
indentation so each policy is valid Kubernetes audit policy YAML.
🪄 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: 070ff4b2-5dbd-4430-8057-ef1fec980558
⛔ Files ignored due to path filters (3)
etcd/vendor/github.com/openshift/microshift/pkg/config/config.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/config/dns.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/util/cryptomaterial/certinfo.gois excluded by!**/vendor/**
📒 Files selected for processing (27)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/bin/common.sh
✅ Files skipped from review due to trivial changes (5)
- assets/optional/metrics-server/kustomization.aarch64.yaml
- test/bin/common.sh
- scripts/auto-rebase/assets_metrics.yaml
- assets/optional/metrics-server/release-metrics-server-x86_64.json
- assets/optional/metrics-server/release-metrics-server-aarch64.json
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/cmd/run.go
- assets/optional/metrics-server/kustomization.x86_64.yaml
- assets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yaml
- assets/optional/metrics-server/00-namespace.yaml
- assets/optional/metrics-server/kustomization.yaml
- assets/optional/metrics-server/01-cluster-role.yaml
- scripts/auto-rebase/assets.yaml
- assets/optional/metrics-server/01-role-binding-auth-reader.yaml
- packaging/observability/microshift-observability.service
- packaging/observability/otelcol.d/microshift-metrics-server.yaml
- assets/optional/metrics-server/04-api-service.yaml
- assets/optional/metrics-server/01-cluster-role-binding.yaml
- pkg/cmd/init.go
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/metrics-server/01-service-account.yaml
- pkg/cmd/metrics.go
- assets/optional/metrics-server/03-deployment.yaml
- packaging/rpm/microshift.spec
- scripts/auto-rebase/rebase.sh
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
2552538 to
6f8a50b
Compare
coderabbitai
Bot
added
the
ready-for-human-review
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
6f8a50b to
82550e8
Compare
coderabbitai
Bot
removed
the
ready-for-human-review
copejon
force-pushed
the
ushift-6951/metrics-server
branch
2 times, most recently
from
9cc9d0e to
261c1a0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/auto-rebase/rebase.sh (1)
1191-1193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not skip unmapped manifest images.
Still valid from the earlier review: warning-and-continue here can emit a partial
kustomization.${arch}.yaml, so the rebase succeeds with broken output.Patch
if [[ -z "${release_tag}" ]]; then - >&2 echo "WARNING: Unknown metrics image '${orig_image}' in ${component_dir}, skipping" - continue + >&2 echo "ERROR: Unknown metrics image '${orig_image}' in ${component_dir}" + return 1 fi🤖 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 `@scripts/auto-rebase/rebase.sh` around lines 1191 - 1193, The current block in rebase.sh that checks if [[ -z "${release_tag}" ]] and then echoes a warning and continues causes partial kustomization.${arch}.yaml output when an image (orig_image) isn't in the mapping; instead, stop skipping: when release_tag is empty, still emit the image into the kustomization entry (use orig_image as the image reference or a sensible fallback) and log the warning but do not use continue; update the code paths that write into kustomization.${arch}.yaml so they handle unmapped images (referencing variables release_tag, orig_image and component_dir) and ensure the resulting kustomization.${arch}.yaml always includes an image entry for every manifest image.
🤖 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 `@scripts/auto-rebase/rebase.sh`:
- Around line 1169-1176: The script assumes jq successfully finds a pullspec;
validate that the jq result is non-empty before writing files: after computing
release_tag and new_image (from the jq command that selects .from.name), check
if new_image is empty (or if new_digest parsed from new_image is empty), and if
so emit a clear error mentioning the release_tag and json_key and exit with a
non-zero status instead of writing component_release_json; apply the same guard
to the other identical block (the one that mirrors lines ~1196-1204) so
malformed release-*.json / kustomization.*.yaml are never created when the
payload tag is missing.
---
Duplicate comments:
In `@scripts/auto-rebase/rebase.sh`:
- Around line 1191-1193: The current block in rebase.sh that checks if [[ -z
"${release_tag}" ]] and then echoes a warning and continues causes partial
kustomization.${arch}.yaml output when an image (orig_image) isn't in the
mapping; instead, stop skipping: when release_tag is empty, still emit the image
into the kustomization entry (use orig_image as the image reference or a
sensible fallback) and log the warning but do not use continue; update the code
paths that write into kustomization.${arch}.yaml so they handle unmapped images
(referencing variables release_tag, orig_image and component_dir) and ensure the
resulting kustomization.${arch}.yaml always includes an image entry for every
manifest image.
🪄 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: 33a078aa-483a-4842-a2dd-2d3d185e3f96
⛔ Files ignored due to path filters (3)
etcd/vendor/github.com/openshift/microshift/pkg/config/config.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/config/dns.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/util/cryptomaterial/certinfo.gois excluded by!**/vendor/**
📒 Files selected for processing (27)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/bin/common.sh
✅ Files skipped from review due to trivial changes (4)
- packaging/observability/microshift-observability.service
- assets/optional/metrics-server/kustomization.yaml
- assets/optional/metrics-server/kustomization.aarch64.yaml
- test/bin/common.sh
🚧 Files skipped from review as they are similar to previous changes (20)
- assets/optional/metrics-server/release-metrics-server-aarch64.json
- assets/optional/metrics-server/01-service-account.yaml
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/metrics-server/01-cluster-role.yaml
- assets/optional/metrics-server/04-api-service.yaml
- assets/optional/metrics-server/00-namespace.yaml
- assets/optional/metrics-server/01-cluster-role-binding.yaml
- assets/optional/metrics-server/02-configmap-audit-profiles.yaml
- assets/optional/metrics-server/04-service.yaml
- pkg/cmd/run.go
- assets/optional/metrics-server/03-deployment.yaml
- packaging/observability/otelcol.d/microshift-metrics-server.yaml
- scripts/auto-rebase/assets_metrics.yaml
- assets/optional/metrics-server/kustomization.x86_64.yaml
- pkg/cmd/init.go
- scripts/auto-rebase/assets.yaml
- packaging/rpm/microshift.spec
- assets/optional/metrics-server/01-role-binding-auth-reader.yaml
- pkg/cmd/metrics.go
- assets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yaml
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
f70198d to
0a515ed
Compare
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
coderabbitai
Bot
added
the
ready-for-human-review
agullon
reviewed
Jun 9, 2026
Contributor
Author
|
@CodeRabbit help |
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
f204ce6 to
9893ecd
Compare
Contributor
Author
|
/test e2e-aws-tests-bootc-periodic-arm-el10 |
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
9893ecd to
01cda48
Compare
coderabbitai
Bot
removed
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/healthcheck/microshift_optional_workloads.go (1)
48-56: ⚡ Quick winConsider defensive slice allocation to avoid backing-array aliasing.
The current implementation appends directly to
existingslices, which may modify shared backing arrays if capacity exists. While correct in current usage, pre-allocating with exact capacity prevents potential aliasing bugs if code changes in the future.♻️ Suggested defensive implementation
func mergeWorkloads(existing, incoming NamespaceWorkloads) NamespaceWorkloads { + deployments := make([]string, 0, len(existing.Deployments)+len(incoming.Deployments)) + deployments = append(deployments, existing.Deployments...) + deployments = append(deployments, incoming.Deployments...) + + daemonSets := make([]string, 0, len(existing.DaemonSets)+len(incoming.DaemonSets)) + daemonSets = append(daemonSets, existing.DaemonSets...) + daemonSets = append(daemonSets, incoming.DaemonSets...) + + statefulSets := make([]string, 0, len(existing.StatefulSets)+len(incoming.StatefulSets)) + statefulSets = append(statefulSets, existing.StatefulSets...) + statefulSets = append(statefulSets, incoming.StatefulSets...) + return NamespaceWorkloads{ - Deployments: append(existing.Deployments, incoming.Deployments...), - DaemonSets: append(existing.DaemonSets, incoming.DaemonSets...), - StatefulSets: append(existing.StatefulSets, incoming.StatefulSets...), + Deployments: deployments, + DaemonSets: daemonSets, + StatefulSets: statefulSets, } }🤖 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/healthcheck/microshift_optional_workloads.go` around lines 48 - 56, mergeWorkloads currently appends incoming slices onto existing slices which can mutate shared backing arrays; change it to allocate new slices with capacity equal to len(existing)+len(incoming) for each field (Deployments, DaemonSets, StatefulSets), copy the existing items into the new slice, then append or copy the incoming items, and return the new NamespaceWorkloads so no original backing arrays are aliased or modified.
🤖 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/healthcheck/microshift_optional_workloads.go`:
- Around line 48-56: mergeWorkloads currently appends incoming slices onto
existing slices which can mutate shared backing arrays; change it to allocate
new slices with capacity equal to len(existing)+len(incoming) for each field
(Deployments, DaemonSets, StatefulSets), copy the existing items into the new
slice, then append or copy the incoming items, and return the new
NamespaceWorkloads so no original backing arrays are aliased or modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1281c4a3-137b-4007-bc64-0b8c9b0cab4d
⛔ Files ignored due to path filters (1)
etcd/vendor/github.com/openshift/microshift/pkg/util/cryptomaterial/certinfo.gois excluded by!**/vendor/**
📒 Files selected for processing (28)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/assets/observability/otel_config.yamltest/bin/common.sh
✅ Files skipped from review due to trivial changes (2)
- assets/optional/metrics-server/release-metrics-server-x86_64.json
- assets/optional/metrics-server/release-metrics-server-aarch64.json
🚧 Files skipped from review as they are similar to previous changes (23)
- pkg/util/cryptomaterial/certinfo.go
- assets/optional/metrics-server/kustomization.yaml
- assets/optional/metrics-server/00-namespace.yaml
- assets/optional/metrics-server/01-cluster-role.yaml
- assets/optional/metrics-server/01-service-account.yaml
- packaging/observability/otelcol.d/microshift-metrics-server.yaml
- scripts/auto-rebase/assets.yaml
- assets/optional/metrics-server/04-service.yaml
- assets/optional/metrics-server/01-cluster-role-binding.yaml
- assets/optional/metrics-server/kustomization.aarch64.yaml
- assets/optional/metrics-server/kustomization.x86_64.yaml
- packaging/observability/microshift-observability.service
- test/bin/common.sh
- assets/optional/metrics-server/02-configmap-audit-profiles.yaml
- pkg/cmd/init.go
- assets/optional/metrics-server/03-deployment.yaml
- assets/optional/metrics-server/04-api-service.yaml
- packaging/rpm/microshift.spec
- pkg/cmd/run.go
- scripts/auto-rebase/assets_metrics.yaml
- pkg/cmd/metrics.go
- assets/optional/metrics-server/01-role-binding-auth-reader.yaml
- scripts/auto-rebase/rebase.sh
Contributor
|
I reviewed the 3 PRs and they look good to me. But at least some basic RF tests are missing. |
Contributor
|
/retest |
pmtk
reviewed
Jun 11, 2026
| if err != nil { | ||
| return fmt.Errorf("creating clientset: %w", err) | ||
| } | ||
| const ns = "openshift-monitoring" |
Member
There was a problem hiding this comment.
Why ns is here, but metricsServerManifestPath outside the function?
Contributor
Author
There was a problem hiding this comment.
both constants are now package-level in a grouped const block, matching the convention in certificateauthority.go.
|
|
||
| const metricsServerManifestPath = "/usr/lib/microshift/manifests.d/080-microshift-metrics-server" | ||
|
|
||
| func provisionMetricsServerCerts(ctx context.Context, cfg *config.Config) error { |
Member
There was a problem hiding this comment.
This looks like it might make a good generic function to create secrets with certs from the disk. Not big deal, just metrics.go seems a bit too specific to be in cmd/. So far I think it's mostly individual commands in each file
Contributor
Author
There was a problem hiding this comment.
I've moved metrics.go to pkg/components and exported provisionMetricsServerCerts, which is now called in pkg/cmd/run.go. Of the 3 metrics components, metrics-server is special in that it requires a kubelet client cert in order to get cluster resource data. Hence the need to generate that cert just before applying the component's manifests.
| // Provision certs for optional components after kustomize creates their namespaces. | ||
| go func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { |
Member
There was a problem hiding this comment.
Do we expect a panic here or is this just very defensive coding?
Contributor
Author
There was a problem hiding this comment.
Defensive, and overkill. Nothing the func can panic. Removed.
| - file: service.yaml | ||
| - file: serviceaccount.yaml | ||
|
|
||
| - dir: optional/metrics-server/ |
Member
There was a problem hiding this comment.
If we have assets_metrics.yaml, then I don't think we should have this entry in this file
Contributor
Author
There was a problem hiding this comment.
Removed the duplicate block — assets_metrics.yaml is the sole tracker. copejon@d61c8be
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add otlp exporter stub to the observability test config so that metrics drop-in configs (which define pipelines exporting to otlp) don't crash otelcol when the test replaces the production config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Strip JSON-style quoted keys from the embedded audit policy documents to match the repo's YAML conventions. Values remain quoted. No functional change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
01cda48 to
0f5a0f3
Compare
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/observability/microshift-observability.service`:
- Line 11: The ExecStart line currently builds a space-joined ARGS string and
then expands it unquoted (variable ARGS) which risks word-splitting/pathname
expansion; change the inline shell to build a bash array (e.g.,
ARGS=("--config=file:/etc/microshift/observability/opentelemetry-collector.yaml")
and append with ARGS+=("--config=file:$f") inside the loop) and finally exec
/usr/bin/opentelemetry-collector "$${ARGS[@]}" to preserve argument boundaries;
update references to ARGS in the ExecStart command accordingly.
In `@scripts/auto-rebase/assets_metrics.yaml`:
- Around line 66-88: The assets_metrics.yaml entry for dir
"optional/kube-state-metrics/" is missing the generated release JSON entries;
update the files list under that dir (near the existing
kustomization.aarch64.yaml / kustomization.x86_64.yaml entries) to add entries
for release-kube-state-metrics-aarch64.json and
release-kube-state-metrics-x86_64.json with ignore: "Provided by MicroShift" so
the rebase.sh generated release files are recognized.
- Around line 39-64: The node-exporter asset block is missing the generated
release JSON entries; update the assets_metrics.yaml node-exporter files list
(near the kustomization.aarch64.yaml and kustomization.x86_64.yaml entries) to
include two new file entries: release-node-exporter-aarch64.json and
release-node-exporter-x86_64.json, each with ignore: "Provided by MicroShift",
so the generated release JSONs are tracked the same way metrics-server does.
🪄 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: 0e9aa2e0-c0af-4de9-ab7b-b053620becdc
⛔ Files ignored due to path filters (1)
etcd/vendor/github.com/openshift/microshift/pkg/util/cryptomaterial/certinfo.gois excluded by!**/vendor/**
📒 Files selected for processing (28)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/assets/observability/otel_config.yamltest/bin/common.sh
✅ Files skipped from review due to trivial changes (4)
- assets/optional/metrics-server/01-service-account.yaml
- assets/optional/metrics-server/release-metrics-server-x86_64.json
- assets/optional/metrics-server/release-metrics-server-aarch64.json
- assets/optional/metrics-server/00-namespace.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- test/assets/observability/otel_config.yaml
- assets/optional/metrics-server/kustomization.x86_64.yaml
- assets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yaml
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/metrics-server/01-cluster-role-binding.yaml
- packaging/observability/otelcol.d/microshift-metrics-server.yaml
- test/bin/common.sh
- assets/optional/metrics-server/04-service.yaml
- assets/optional/metrics-server/kustomization.yaml
- assets/optional/metrics-server/01-cluster-role.yaml
- assets/optional/metrics-server/04-api-service.yaml
- assets/optional/metrics-server/01-role-binding-auth-reader.yaml
- pkg/cmd/run.go
- assets/optional/metrics-server/02-configmap-audit-profiles.yaml
- pkg/cmd/metrics.go
- pkg/cmd/init.go
- assets/optional/metrics-server/03-deployment.yaml
- scripts/auto-rebase/assets.yaml
- packaging/rpm/microshift.spec
- scripts/auto-rebase/rebase.sh
Contributor
Author
|
2 unrelated flakes resulted in failures /test e2e-aws-tests |
…ntries Use a bash array for otelcol config arguments to prevent word-splitting, and add generated release JSON entries for node-exporter and kube-state-metrics in assets_metrics.yaml to match metrics-server. Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
coderabbitai
Bot
removed
the
ready-for-human-review
…er fit for ./pkg/components/. components.ProvisionMetricsServerCerts must still be called from ./pkg/cmd/run.go because the metrics-server needs its client tls certs on host prior to deploying the component
coderabbitai
Bot
added
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 `@pkg/components/metrics.go`:
- Around line 82-85: When handling AlreadyExists for the
metrics-server-client-certs Secret and kubelet-serving-ca-bundle ConfigMap in
pkg/components/metrics.go, preserve metadata.resourceVersion before calling
Update: fetch the existing Secret/ConfigMap (or inspect the returned error to
Get the current object), copy its ObjectMeta.ResourceVersion into the
secret.ObjectMeta.ResourceVersion / configMap.ObjectMeta.ResourceVersion, then
call clientset.CoreV1().Secrets(...).Update(...) or
clientset.CoreV1().ConfigMaps(...).Update(...). This ensures Kubernetes accepts
the Update and TLS material can rotate/reconcile.
🪄 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: 47f8b340-9498-4acf-9808-35a8ab10d668
📒 Files selected for processing (2)
pkg/cmd/run.gopkg/components/metrics.go
Comment on lines
+82
to
+85
| _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Create(ctx, secret, metav1.CreateOptions{}) | ||
| if apierrors.IsAlreadyExists(err) { | ||
| _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Update(ctx, secret, metav1.UpdateOptions{}) | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve resourceVersion on Secret/ConfigMap Update so TLS material can reconcile
In pkg/components/metrics.go, the AlreadyExists path for the metrics-server-client-certs Secret (around 82-85) and the kubelet-serving-ca-bundle ConfigMap (around 107-110) calls Update with objects that omit metadata.resourceVersion; Kubernetes Update requires it, so rotation won’t apply after the first creation (even though these are mounted by assets/optional/metrics-server/03-deployment.yaml).
Suggested fix
- _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Create(ctx, secret, metav1.CreateOptions{})
- if apierrors.IsAlreadyExists(err) {
- _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Update(ctx, secret, metav1.UpdateOptions{})
- }
+ _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Create(ctx, secret, metav1.CreateOptions{})
+ if apierrors.IsAlreadyExists(err) {
+ existing, getErr := clientset.CoreV1().Secrets(metricsServerNamespace).Get(ctx, secret.Name, metav1.GetOptions{})
+ if getErr != nil {
+ return fmt.Errorf("getting existing metrics-server client cert secret: %w", getErr)
+ }
+ secret.ResourceVersion = existing.ResourceVersion
+ _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Update(ctx, secret, metav1.UpdateOptions{})
+ }
if err != nil {
return fmt.Errorf("applying metrics-server client cert secret: %w", err)
}
@@
- _, err = clientset.CoreV1().ConfigMaps(metricsServerNamespace).Create(ctx, cm, metav1.CreateOptions{})
- if apierrors.IsAlreadyExists(err) {
- _, err = clientset.CoreV1().ConfigMaps(metricsServerNamespace).Update(ctx, cm, metav1.UpdateOptions{})
- }
+ _, err = clientset.CoreV1().ConfigMaps(metricsServerNamespace).Create(ctx, cm, metav1.CreateOptions{})
+ if apierrors.IsAlreadyExists(err) {
+ existing, getErr := clientset.CoreV1().ConfigMaps(metricsServerNamespace).Get(ctx, cm.Name, metav1.GetOptions{})
+ if getErr != nil {
+ return fmt.Errorf("getting existing kubelet serving CA configmap: %w", getErr)
+ }
+ cm.ResourceVersion = existing.ResourceVersion
+ _, err = clientset.CoreV1().ConfigMaps(metricsServerNamespace).Update(ctx, cm, metav1.UpdateOptions{})
+ }
if err != nil {
return fmt.Errorf("applying kubelet serving CA configmap: %w", err)
}📝 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
| _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Create(ctx, secret, metav1.CreateOptions{}) | |
| if apierrors.IsAlreadyExists(err) { | |
| _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Update(ctx, secret, metav1.UpdateOptions{}) | |
| } | |
| _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Create(ctx, secret, metav1.CreateOptions{}) | |
| if apierrors.IsAlreadyExists(err) { | |
| existing, getErr := clientset.CoreV1().Secrets(metricsServerNamespace).Get(ctx, secret.Name, metav1.GetOptions{}) | |
| if getErr != nil { | |
| return fmt.Errorf("getting existing metrics-server client cert secret: %w", getErr) | |
| } | |
| secret.ResourceVersion = existing.ResourceVersion | |
| _, err = clientset.CoreV1().Secrets(metricsServerNamespace).Update(ctx, secret, metav1.UpdateOptions{}) | |
| } |
🤖 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/components/metrics.go` around lines 82 - 85, When handling AlreadyExists
for the metrics-server-client-certs Secret and kubelet-serving-ca-bundle
ConfigMap in pkg/components/metrics.go, preserve metadata.resourceVersion before
calling Update: fetch the existing Secret/ConfigMap (or inspect the returned
error to Get the current object), copy its ObjectMeta.ResourceVersion into the
secret.ObjectMeta.ResourceVersion / configMap.ObjectMeta.ResourceVersion, then
call clientset.CoreV1().Secrets(...).Update(...) or
clientset.CoreV1().ConfigMaps(...).Update(...). This ensures Kubernetes accepts
the Update and TLS material can rotate/reconcile.
Contributor
Author
|
Ah, need to re-rebase. Handling that now |
Contributor
|
@copejon: The following tests 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 metrics-server as an optional MicroShift component deployed via kustomize manifests and packaged as
microshift-metrics-serverandmicroshift-metrics-server-release-infoRPM sub-packages.This is PR 1/3 splitting #6763 into independently-mergeable patches. The three PRs can merge in any order.
Siblings: #6809 (kube-state-metrics), #6810 (node-exporter)
What's included
assets/optional/metrics-server/) — Deployment, APIService, RBAC, NetworkPolicy, PDB, service-ca TLS, andopenshift-monitoringnamespacepkg/cmd/metrics.go)mergeWorkloads()function and metrics-server map entry in optional workload paths%package metrics-serverand%package metrics-server-release-infowith per-file installsotelcol.d/directory structure andmicroshift-metrics-server.yamlscrape config; service file updated to load drop-in configs via bash wrapperupdate_metrics_images()inrebase.sh+assets_metrics.yamlfor all three exporters (shared infrastructure lives in this PR)test/bin/common.shSummary by CodeRabbit
Release Notes
New Features
Chores