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.

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

Metrics "Sidecar" (scraping container) support #3504

Merged
merged 21 commits into from
Jun 17, 2019

Conversation

jeefy
Copy link
Member

@jeefy jeefy commented Jan 10, 2019

WIP

This is the Dashboard side for using the metrics server. Fixes #2986

  • Functional Support (through the additional container)
  • Finish rebasing to "v2.0.0-alpha"
  • Official Repo for the scraping container (now at https://github.com/kubernetes-sigs/dashboard-metrics-scraper)
  • Docker Hub dev registry for the scraping container
  • Update head deploy scripts to include scraping container

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 10, 2019
@jeefy
Copy link
Member Author

jeefy commented Jan 10, 2019

/hold

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 10, 2019
@k8s-ci-robot k8s-ci-robot requested review from ianlewis and PeWu January 10, 2019 14:32
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2019
@jeefy
Copy link
Member Author

jeefy commented Jan 10, 2019

/approve cancel

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2019
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #3504 into master will increase coverage by 0.7%.
The diff coverage is 68.35%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3504     +/-   ##
=========================================
+ Coverage   46.57%   47.28%   +0.7%     
=========================================
  Files         173      181      +8     
  Lines        8066     8389    +323     
  Branches       59       71     +12     
=========================================
+ Hits         3757     3967    +210     
- Misses       4079     4178     +99     
- Partials      230      244     +14
Impacted Files Coverage Δ
.../app/backend/integration/metric/heapster/client.go 74.38% <ø> (ø) ⬆️
src/app/backend/integration/metric/manager.go 50% <0%> (-8.7%) ⬇️
...p/backend/integration/metric/sidecar/restclient.go 0% <0%> (ø)
...c/app/backend/integration/metric/sidecar/common.go 100% <100%> (ø)
...app/backend/integration/metric/sidecar/selector.go 70.31% <70.31%> (ø)
...c/app/backend/integration/metric/sidecar/client.go 74.38% <74.38%> (ø)
...rc/app/backend/integration/metric/sidecar/model.go 77.77% <77.77%> (ø)
...p/backend/integration/metric/common/aggregation.go 89.09% <0%> (-1.82%) ⬇️
src/app/backend/handler/filter.go 45.45% <0%> (-1.39%) ⬇️
src/app/frontend/overview/helper.ts 89.47% <0%> (-0.53%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f661b8a...bbbbc84. Read the comment docs.

@jeefy
Copy link
Member Author

jeefy commented Mar 21, 2019

I'm not going to block on the gcr.io repo at this point.

At this point I'm good moving this into master, tackling any emergent bugs, and getting people testing both.

To test, people will have to use aio/deploy/recommended/kubernetes-dashboard-head.yaml -- However, head will test both head of Dashboard, as well as head of metrics-scraper.

🎉

/hold cancel
/assign @maciaszczykm @floreks

@jeefy jeefy changed the title [WIP] Metrics "Sidecar" (scraping container) support Metrics "Sidecar" (scraping container) support Mar 21, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2019
@jeefy
Copy link
Member Author

jeefy commented Mar 21, 2019

/hold cancel (I can't prow sometimes)

@jeefy jeefy removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2019
@jeefy
Copy link
Member Author

jeefy commented Jun 12, 2019

@maciaszczykm @floreks I think it's time for a review.

In this PR I also added a "local" deployment YAML. For reference, here's how I just tested it.

minikube start
minikube addons list # ensure metrics-server addon is enabled
eval $(minikube docker-env)
npm run docker:build:head
kubectl apply -f aio/deploy/recommended/kubernetes-dashboard-local.yaml
kubectl proxy

And then viewing the dashboard via kubectl proxy. Let me know if there's anything else needed to dive into this.

Copy link
Member

@maciaszczykm maciaszczykm left a comment

Choose a reason for hiding this comment

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

/approve

Let's wait for @floreks with LGTM.

@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 13, 2019
@floreks
Copy link
Member

floreks commented Jun 13, 2019

I wouldn't add this local deployment file to our aio/deploy directory as it should contain official deployments. From my understanding, this deployment will only work if we first build an image locally and then try to run it.

If we want to keep it then I'd move it maybe to a test-resources dir, rename it to something like metrics-test-deployment.yaml and also rename dashboard deployment inside the file so it does not collide with the official head deployment.

@jeefy @maciaszczykm WDYT?

@maciaszczykm
Copy link
Member

@floreks SGTM.

@jeefy
Copy link
Member Author

jeefy commented Jun 13, 2019

Sounds good, I'll update the PR later this morning.

@pires
Copy link

pires commented Jun 13, 2019

@jeefy I'm here rooting for you. What a champ you have been with this PR 🙇

@pierluigilenoci
Copy link
Contributor

@ianlewis @PeWu please take a look 😘

@maciaszczykm
Copy link
Member

LGTM, cc @floreks.

@floreks
Copy link
Member

floreks commented Jun 17, 2019

Let's get this in! 🍾

/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 17, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, maciaszczykm

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:
  • OWNERS [floreks,maciaszczykm]

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 merged commit 5c5c736 into kubernetes:master Jun 17, 2019
@jeefy
Copy link
Member Author

jeefy commented Jun 17, 2019

Thanks for the support y'all!

image

@eloyekunle
Copy link
Contributor

Thanks @jeefy

@jseguillon
Copy link

Wow @jeefy you're the boss ! Thanks !

@RainingNight
Copy link

Good!

@tdmalone
Copy link

Thanks so much for this @jeefy! Just to clarify, once a new version is cut (although I note it has been last year since this happened...), this means heapster won't be required anymore and Dashboard will be able to display metrics from metrics-server?

@jeefy
Copy link
Member Author

jeefy commented Jun 18, 2019

@tdmalone Correct. Dashboard will (now) deploy two containers, the dashboard itself and the Dashboard Metrics Scraper. Since metrics-server only shows a single moment of time in the cluster, the scraper is needed to poll/store the metrics for look-up. The next thing to tackle is being able to point the Dashboard at Prometheus, but that'll be easier to do after the efforts of our GSOC students (Yay!)

Also, we should be cutting an RC at the end of this month, but you can (technically) spin up the Dashboard off the master branch using the kubernetes-dashboard-head.yaml deployment.

Just keep in mind, that's literally a build off master, not an actual release, there be dragons there, etc etc. 😄

@varuzam
Copy link

varuzam commented Jun 18, 2019

I run the kubernetes dashboard with the manifest https://gist.githubusercontent.com/varuzam/d624547b4286dd11206e1fc0a442f78d/raw/579905246348002b7b9895c60b3ae996de3f664a/kube-dashboard.yaml
The dashboard can not get metrics from the scrapper. Here is logs of the scrapper:

{"level":"info","msg":"Database updated: 2 nodes, 7 pods","time":"2019-06-18T19:05:06Z"}
{"level":"info","msg":"URL: /","time":"2019-06-18T19:05:14Z"}
{"level":"info","msg":"URL: /","time":"2019-06-18T19:05:24Z"}
{"level":"info","msg":"URL: /api/v1/api/v1/dashboard/namespaces/kube-system/pod-list/kubernetes-dashboard-86df9bb89b-l2226,metrics-server-54fddf7759-t8nnr,coredns-58b4966dd4-5gjxx,coredns-58b4966dd4-p4cn6/metrics/cpu/usage_rate","time":"2019-06-18T19:05:30Z"}

It looks like the dashboard uses wrong url with doubled '/api/v1'

@floreks
Copy link
Member

floreks commented Jun 19, 2019

@varuzam how did you run the scraper? In or outside of the cluster? It does work when both Dashboard and the scraper are running inside. There is an issue if I am trying to access it using remote option.

@jeefy

@varuzam
Copy link

varuzam commented Jun 19, 2019

@floreks I run the scraper in the one pod with the dashboard inside a cluster.
My deployment - https://gist.githubusercontent.com/varuzam/d624547b4286dd11206e1fc0a442f78d/raw/579905246348002b7b9895c60b3ae996de3f664a/kube-dashboard.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Support metrics API