New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kubelet: add support for broadcasting metrics from CRI #113609
Conversation
/sig node |
I was able to get a successful e2e by using https://github.com/haircommander/cri-o/tree/metrics-wip and this. Running with a local cluster, I could curl the Kubelet's HTTP endpoint |
4497b49
to
0ae21d7
Compare
|
||
message Metric { | ||
string name = 1; | ||
string help = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure we want help here? This is a bit unfortunate because it means we need to send help text for every single metric when help is the same for the same metric "name". It seems like it will increase the rpc payload size alot due to the duplication (since the metric is per container).
Maybe we can omit this for now and figure out best way to include this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work :)
0ae21d7
to
ef4e749
Compare
ef4e749
to
34fa716
Compare
a couple of highlights to the updates:
PTAL @dashpole @bobbypage |
5f6bd86
to
7a71cbf
Compare
cc338fd
to
95df62f
Compare
thanks for the review @bobbypage, comments addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
we need to measure performance before enabling beta.
r.RawMustRegister(metrics.NewPrometheusMachineCollector(prometheusHostAdapter{s.host}, includedMetrics)) | ||
if utilfeature.DefaultFeatureGate.Enabled(features.PodAndContainerStatsFromCRI) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks sufficiently isolated to make it safe for alpha.
all the code falls back to existing behavior when not enabled.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, derekwaynecarr, haircommander The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/triage accepted |
Thank you for the updates! /lgtm |
95df62f
to
c20b002
Compare
so that a caller can use the metrics.Metric structure but still handle errors Signed-off-by: Peter Hunt <pehunt@redhat.com>
Added new gRPC call 'ListPodSanboxMetrics' which would return additional container stats currently supported by cAdvisor, but outside the scope of /stats/summary api. Added new types to support metric exporting of prometheus, including Metric and other subfields. Added fake runtime changes associated with the CRI changes.
Signed-off-by: Peter Hunt <pehunt@redhat.com>
that pulls metrics from the CRI Signed-off-by: Peter Hunt <pehunt@redhat.com>
Signed-off-by: Peter Hunt <pehunt@redhat.com>
c20b002
to
95489a2
Compare
re-apply LGTM /lgtm |
/lgtm |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR includes #113025 as well as support in the kubelet for pulling the metrics from CRI and broadcasting them to prometheus
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
There are two open questions that I need someone who knows more about the way these endpoints work:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: