USHIFT-6951: Add metrics-server as optional MicroShift component#6808
Open
copejon wants to merge 18 commits into
Open
USHIFT-6951: Add metrics-server as optional MicroShift component#6808copejon wants to merge 18 commits into
copejon wants to merge 18 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:
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 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
7b1cdef to
2c93779
Compare
Member
|
/lgtm |
Member
|
/lgtm cancel |
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: copejon, pmtk 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 |
Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
use resourceapply for idempotent secret/configmap apply with retry, return (false, nil) on transient API errors so polling continues, propagate errors synchronously instead of fire-and-forget goroutine Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
add KubeletServingCAPath containing only kubelet-signer and kube-csr-signer; use it instead of the broader KubeletClientCAPath Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
2c93779 to
8aea5c2
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
Contributor
Author
|
/retest |
pacevedom
reviewed
Jun 18, 2026
Comment on lines
+374
to
+377
| ).WithCABundle( | ||
| cryptomaterial.KubeletServingCAPath(certsDir), | ||
| []string{"kubelet-signer"}, | ||
| []string{"kubelet-signer", "kube-csr-signer"}, |
Contributor
There was a problem hiding this comment.
wondering if this would fit in the previous CA bundle, there is only 1 difference and its just CAs.
|
|
||
| var metricsEventRecorder events.Recorder = events.NewLoggingEventRecorder("microshift-metrics-server", clock.RealClock{}) | ||
|
|
||
| func ProvisionMetricsServerCerts(ctx context.Context, cfg *config.Config) error { |
Contributor
There was a problem hiding this comment.
Wondering if this function should be asynchronous. For an optional component it would block MicroShift readiness until its ready, and this is the only component doing this that I know of.
Contributor
Author
There was a problem hiding this comment.
ah that's a good point. will fix
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agullon
reviewed
Jun 19, 2026
| // Provision certs for optional components after kustomize creates their namespaces. | ||
| if err := components.ProvisionMetricsServerCerts(runCtx, cfg); err != nil { | ||
| return fmt.Errorf("failed to provision metrics-server certs: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
hi @copejon if I understand this block correctly, it blocks the go routine until the metrics server is installed and running. Also if there's any failure it reports microshift failed to start, right? Is this the desired behavior? It looks to me an overkill to report microshift failure if an extra component instalation fails.
agullon
reviewed
Jun 19, 2026
| err = wait.PollUntilContextTimeout(ctx, 2*time.Second, 1*time.Minute, true, func(ctx context.Context) (bool, error) { | ||
| _, _, err := resourceapply.ApplyConfigMap(ctx, clientset.CoreV1(), metricsEventRecorder, cm) | ||
| if err != nil { | ||
| return false, fmt.Errorf("applying kubelet serving CA configmap: %w", err) |
Contributor
There was a problem hiding this comment.
hi @copejon, I'm comparing the error handeling of this ConfigMap apply function with the previous Secret apply function and I see few differences I don't quite understand, can you please review it?
- Secret in line 92 does not return error only log the error, but ConfigMap on line 122 returns the error.
- Secret in line 98 return error with
%wand ConfigMap in line 127 return error with%v
Remove otelcol drop-in config, revert observability service ExecStart to single-config mode, and drop otelcol-related lines from the RPM spec. Observability integration is deferred to a follow-up PR. Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
Move MS asset tracking and image update logic out of the shared rebase.sh. Delete the separate assets_metrics.yaml and update presubmit.py to reference the new unified asset file. 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>
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
8aea5c2 to
e6f9dc8
Compare
Contributor
Author
|
Scheduling failures blocking some of the tests from starting: /test e2e-aws-tests-bootc-el10 |
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
5 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