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

Instrument the Azure API calls for Prometheus monitoring #58204

Merged
merged 3 commits into from
Jan 15, 2018
Merged

Instrument the Azure API calls for Prometheus monitoring #58204

merged 3 commits into from
Jan 15, 2018

Conversation

ccojocar
Copy link
Member

What this PR does / why we need it:

Instruments the Azure API calls for Prometheus monitoring.

Special notes for your reviewer:

This is version 2 based on the wrapped clients.

Release note:

Instrument the Azure cloud provider for Prometheus monitoring.

cc @feiskyer @andyzhangx @jdumars @khenidak

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 12, 2018
@ccojocar ccojocar mentioned this pull request Jan 12, 2018
Closed
@andyzhangx
Copy link
Member

/ok-to-test
/sig azure

@k8s-ci-robot k8s-ci-robot added sig/azure and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 12, 2018
@andyzhangx
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2018
feiskyer
feiskyer reviewed Jan 14, 2018
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

@cosmincojocar Seems all ListNextResults interfaces are not instrumented, why not also add them?

@ccojocar
Copy link
Member Author

ccojocar commented Jan 14, 2018 •
edited
Loading

Seems all ListNextResults interfaces are not instrumented, why not also add them?

@feiskyer That's right. It isn't obvious to me from where to read the resource group without modifying the interfaces. It doesn't appear to be available in the current object.

One possibility is to add the resource group in the ListNextRresults method signatures. Do you have any other suggestions?

@feiskyer
Copy link
Member

One possibility is to add the resource group in the ListNextRresults method signatures. Do you have any other suggestions?

I thought resource group is included in lastResult, but it doesn't really after checking codes. I'm ok with add it in ListNextResults() signatures.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2018
@ccojocar
Copy link
Member Author

/retest

andyzhangx
andyzhangx reviewed Jan 15, 2018
View reviewed changes
pkg/cloudprovider/providers/azure/azure_metrics.go Outdated
@@ -0,0 +1,82 @@
/*
Copyright 2017 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

2018

andyzhangx
andyzhangx reviewed Jan 15, 2018
View reviewed changes
pkg/cloudprovider/providers/azure/azure_client.go Outdated
@@ -84,7 +84,7 @@ type VirtualMachineScaleSetsClient interface {
CreateOrUpdate(resourceGroupName string, VMScaleSetName string, parameters compute.VirtualMachineScaleSet, cancel <-chan struct{}) (<-chan compute.VirtualMachineScaleSet, <-chan error)
Get(resourceGroupName string, VMScaleSetName string) (result compute.VirtualMachineScaleSet, err error)
List(resourceGroupName string) (result compute.VirtualMachineScaleSetListResult, err error)
ListNextResults(lastResults compute.VirtualMachineScaleSetListResult) (result compute.VirtualMachineScaleSetListResult, err error)
ListNextResults(resourceGroupName string, astResults compute.VirtualMachineScaleSetListResult) (result compute.VirtualMachineScaleSetListResult, err error)
Copy link
Member

Choose a reason for hiding this comment

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

lastResults

@andyzhangx
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2018
@feiskyer
Copy link
Member

/test pull-kubernetes-e2e-gce-device-plugin-gpu

feiskyer
feiskyer approved these changes Jan 15, 2018
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/approve no-issue

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, cosmincojocar, feiskyer

Associated issue requirement bypassed by: feiskyer

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your 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 Jan 15, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 590d9fc into kubernetes:master Jan 15, 2018
@djsly
Copy link
Contributor

djsly commented Apr 23, 2018

Quick question, what is the endpoint for having our prometheus stack scrape the cloud-provider metrics ?

Is this done automatically but some kubernetes' internal? Thanks!

@djsly
Copy link
Contributor

djsly commented Apr 23, 2018

FYI, I’m stil using the integrated cloud-provider and not the external cloud-provider-manager. So can we access the cloud-provider endpoint when running the integrated version in kubelet ?

@djsly
Copy link
Contributor

djsly commented Apr 27, 2018

@cosmincojocar any idea ?

@omerlh
Copy link

omerlh commented Feb 17, 2019

@djsly did you find an answer? @ccojocar I just installed 1.10 on Azure and I don't see these metrics, should I add a job?

@djsly
Copy link
Contributor

djsly commented Feb 18, 2019

@omerlh , I think you need to use the external cloud provider for access the /metrics, something that I'm not using since the azure cloud provider is still integrated within the upstream code

@omerlh
Copy link

omerlh commented Feb 18, 2019

Ok, I hope someone from sig azure will be able to answer that question...

@feiskyer
Copy link
Member

@omerlh You could get them from kube-controller-manager's metrics API.

@omerlh
Copy link

omerlh commented Feb 19, 2019

So I should added it the annonation so prometheus will scrape it?

@omerlh
Copy link

omerlh commented Feb 19, 2019

So I need to add an annotation to kube-controller-manager in order for prometheus to scrape it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyzhangx andyzhangx andyzhangx left review comments

@feiskyer feiskyer feiskyer approved these changes

Assignees

@jdumars jdumars

@andyzhangx andyzhangx

@karataliu karataliu

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ccojocar @andyzhangx @feiskyer @k8s-ci-robot @k8s-github-robot @djsly @omerlh @jdumars @karataliu