Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement using /metrics/resource Kubelet endpoint #777

Merged
merged 1 commit into from
Jun 28, 2021
Merged

Implement using /metrics/resource Kubelet endpoint #777

merged 1 commit into from
Jun 28, 2021

Conversation

yangjunmyfm192085
Copy link
Contributor

Signed-off-by: JunYang [email protected]

What this PR does / why we need it:
Implement using /metrics/resource Kubelet endpoint
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #559

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 27, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 27, 2021
@yangjunmyfm192085
Copy link
Contributor Author

/cc @serathius @dgrisonnet

@k8s-ci-robot k8s-ci-robot requested a review from dgrisonnet May 27, 2021 11:56
serathius
serathius reviewed May 28, 2021
View reviewed changes
manifests/base/rbac.yaml Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 29, 2021
@yangjunmyfm192085 yangjunmyfm192085 force-pushed the metrics-server-test8 branch 2 times, most recently from 60a298b to c6398d6 Compare May 29, 2021 16:49
@yangjunmyfm192085
Copy link
Contributor Author

@serathius I have fixed all. Could you take a look?

serathius
serathius reviewed May 29, 2021
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@yangjunmyfm192085 yangjunmyfm192085 force-pushed the metrics-server-test8 branch 12 times, most recently from 087a07a to 8e26cd2 Compare June 25, 2021 03:07
@yangjunmyfm192085
Copy link
Contributor Author

@serathius, I have been trying but it doesn't seem to work.Maybe the data content in the file is too different, So the file is recognized as deleted and added instead of moved, Is there a good way to modify?

serathius
serathius reviewed Jun 25, 2021
View reviewed changes
pkg/scraper/client/resource/decode.go Outdated
for containerName, containerMetric := range podMetric.Containers {
if containerMetric != (storage.MetricsPoint{}) {
//drop metrics when Timestamp is zero
if containerMetric.Timestamp.IsZero() || containerMetric.CumulativeCpuUsed == 0 || containerMetric.MemoryUsage == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vs previous code, this removes the need for container.StartTime to be provided. Can you check if Storage will handle case with StartTime zero, is there a test for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I updated the test cases.

@serathius
Copy link
Contributor

Overall this looks ok for me.
/approve

Would also want an LGTM from @dgrisonnet.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: serathius, yangjunmyfm192085

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2021
@yangjunmyfm192085
Copy link
Contributor Author

/cc dgrisonnet

@serathius
Copy link
Contributor

/assign @dgrisonnet

dgrisonnet
dgrisonnet reviewed Jun 26, 2021
View reviewed changes
Copy link
Member

@dgrisonnet dgrisonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two comments from my side, but it looks good to me otherwise.

pkg/scraper/client/resource/decode.go Outdated Show resolved Hide resolved
pkg/scraper/client/resource/decode.go Outdated Show resolved Hide resolved
@yangjunmyfm192085
Copy link
Contributor Author

Hi, @dgrisonnet, thanks for your review, I have applied all your suggestion. Could you take a look again?

@dgrisonnet
Copy link
Member

Awesome, thanks @yangjunmyfm192085.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit b28fa66 into kubernetes-sigs:master Jun 28, 2021
@dfr dfr mentioned this pull request Jan 26, 2023
Open
@pacoxu pacoxu mentioned this pull request Sep 4, 2023
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serathius serathius serathius left review comments

@dgrisonnet dgrisonnet dgrisonnet left review comments

@s-urbaniak s-urbaniak Awaiting requested review from s-urbaniak

Assignees

@dgrisonnet dgrisonnet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.6
Development

Successfully merging this pull request may close these issues.

Implement using /metrics/resource Kubelet endpoint
5 participants
@yangjunmyfm192085 @serathius @k8s-ci-robot @dgrisonnet @bbarnes52-zz