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

Update CNI metrics #413

Merged
merged 3 commits into from
May 2, 2019
Merged

Update CNI metrics #413

merged 3 commits into from
May 2, 2019

Conversation

mogren
Copy link
Contributor

@mogren mogren commented Apr 25, 2019 •
edited
Loading

*Resolves issue #113 and #409 *

Description of changes:

  • Replacing Add cni-metrics-helper source code #88 with an up to date PR. (To see diff against Add cni-metrics-helper source code #88, look at c8faee2)
  • Renamed prometheus endpoints to match the updated names in the CNI
  • Made unit tests actually run
  • Updated Makefile to include metrics target
  • Made a dockerized metrics build
  • Added awscni_add_ip_req_count and awscni_del_ip_req_count
  • Made metrics agent version number match CNI release
  • Made USE_CLOUDWATCH="yes" the default
  • Passes go vet
  • If the CLUSTER_ID tag is not set on the node, try Name before defaulting to k8s-cluster

Examples
To apply against CNI v1.4.0:

kubectl apply -f https://raw.githubusercontent.com/aws/amazon-vpc-cni-k8s/release-1.4/config/v1.4/cni_metrics_helper.yaml

Check logs:

> kubectl get pods --all-namespaces  
NAMESPACE     NAME                                 READY   STATUS    RESTARTS   AGE
kube-system   aws-node-q64fb                       1/1     Running   0          4h
kube-system   cni-metrics-helper-9f9d65f97-h5tq5   1/1     Running   0          2h
kube-system   coredns-7d77776957-lx6fr             1/1     Running   0          17d
kube-system   kube-proxy-rvbkh                     1/1     Running   0          17d

> kubectl logs -n kube-system cni-metrics-helper-9f9d65f97-h5tq5

.
.
I0425 21:28:34.234286       1 metrics.go:329] Produce COUNTER metrics: awsAPIErr, value: 5.000000
I0425 21:28:34.234295       1 metrics.go:339] Produce GAUGE metrics: ipamdActionInProgress, value: 0.000000
I0425 21:28:34.234301       1 metrics.go:339] Produce GAUGE metrics: assignIPAddresses, value: 3.000000
I0425 21:28:34.234307       1 metrics.go:339] Produce GAUGE metrics: maxIPAddresses, value: 45.000000
I0425 21:28:34.234315       1 metrics.go:329] Produce COUNTER metrics: addReqCount, value: 161.000000
I0425 21:28:34.234324       1 metrics.go:329] Produce COUNTER metrics: delReqCount, value: 190.000000
I0425 21:28:34.234331       1 metrics.go:339] Produce GAUGE metrics: eniAllocated, value: 4.000000
I0425 21:28:34.234335       1 metrics.go:339] Produce GAUGE metrics: eniMaxAvailable, value: 9.000000
I0425 21:28:34.234341       1 metrics.go:339] Produce GAUGE metrics: totalIPAddresses, value: 20.000000

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mogren mogren added this to the v1.5 milestone Apr 25, 2019
This was referenced Apr 25, 2019
Closed
Closed
Closed
@mogren mogren requested a review from vsiddharth April 29, 2019 17:04
@StevenACoffman StevenACoffman mentioned this pull request Apr 29, 2019
Closed
vsiddharth
vsiddharth suggested changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@vsiddharth vsiddharth left a comment

Choose a reason for hiding this comment

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

Added initial set of comments for the second commit.

cni-metrics-helper/cni-metrics-helper.go
@@ -0,0 +1,102 @@
package main
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license headers

cni-metrics-helper/metrics/cni_metrics.go Outdated
@@ -0,0 +1,153 @@
// Package metrics handles the processing of all metrics. This file handles metrics for kube-state-metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Add license headers

cni-metrics-helper/metrics/cni_metrics.go Outdated
// CNIMetricsTarget defines data structure for kube-state-metric target
type CNIMetricsTarget struct {
interestingMetrics map[string]metricsConvert
cwClient publisher.Publisher
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/cwClient/cwMetricsPublisher/

cni-metrics-helper/metrics/cni_metrics.go Outdated

func (t *CNIMetricsTarget) grabMetricsFromTarget(cniPod string) ([]byte, error) {

glog.Info("Grabbing metrics from CNI ", cniPod)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/Info/Infof

cni-metrics-helper/metrics/cni_metrics.go Outdated
func (t *CNIMetricsTarget) grabMetricsFromTarget(cniPod string) ([]byte, error) {

glog.Info("Grabbing metrics from CNI ", cniPod)
output, err := getMetricsFromPod(t.kubeClient, cniPod, metav1.NamespaceSystem, 61678)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make 61678 a const

cni-metrics-helper/metrics/cni_metrics.go
@@ -12,78 +12,92 @@ import (

// InterestingCNIMetrics defines metrics parsing definition for kube-state-metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Metrics from cni prometheus endpoint

cni-metrics-helper/metrics/metrics.go
@@ -1,4 +1,4 @@
// Package metrics provide common data structure and routines for converting/aggregating prometheus metrics to cloudwatch metrics
// Package metrics provide common data structure and routines for converting/aggregating prometheus metrics to cloudwatch metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add license headers

cni-metrics-helper/metrics/metrics.go Outdated
@@ -327,7 +318,7 @@ func filterMetrics(originalMetrics map[string]*dto.MetricFamily,
}

func produceCloudWatchMetrics(t metricsTarget, families map[string]*dto.MetricFamily, convertDef map[string]metricsConvert, cw publisher.Publisher) {
// TODO the following will be changed to integate with cloudwatch publisher
// TODO: The following will be changed to integrate with CloudWatch publisher
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove comment

pkg/ec2wrapper/ec2wrapper.go
@@ -8,6 +8,7 @@ import (
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
"github.com/golang/glog"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is glog used within this pkg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

pkg/publisher/publisher.go
@@ -1,4 +1,4 @@
// Package publisher is used to batch and send metric data to cloudwatch
// Package publisher is used to batch and send metric data to CloudWatch
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add license headers

@mogren mogren requested a review from vsiddharth May 1, 2019 00:27
vsiddharth
vsiddharth approved these changes May 2, 2019
View reviewed changes
Copy link
Contributor

@vsiddharth vsiddharth left a comment

Choose a reason for hiding this comment

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

@mogren Changes LGTM :shipit:

Please have follow up items to address issues with the original commits!

@mogren mogren merged commit c34df10 into aws:master May 2, 2019
@tiffanyfay tiffanyfay mentioned this pull request May 3, 2019
Merged
@mogren mogren deleted the update-cni-metrics branch May 23, 2019 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsiddharth vsiddharth vsiddharth approved these changes

@nckturner nckturner Awaiting requested review from nckturner

@tiffanyfay tiffanyfay Awaiting requested review from tiffanyfay

Assignees
No one assigned
Labels
enhancement tech debt
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.
3 participants
@mogren @vsiddharth @liwenwu-amazon