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

Add cni-metrics-helper source code #88

Closed
wants to merge 2 commits into from

Conversation

liwenwu-amazon
Copy link
Contributor

@liwenwu-amazon liwenwu-amazon commented May 28, 2018

*Issue #113 *

Description of changes:

cni-metrics-helper

The following show how cni-metrics-helper works in a cluster:

cni-metrics-helper

install cni-metrics-helper

kubectl apply -f misc/cni_metrics_helper.yaml

get cni-metrics-helper logs

kubectl get pod -n kube-system
NAME                                  READY     STATUS    RESTARTS   AGE 
aws-node-248ns                        1/1       Running   0          6h  
aws-node-257bn                        1/1       Running   0          2h  
...
cni-metrics-helper-6dcff5ddf4-v5l6d   1/1       Running   0          7h  
kube-dns-75fddcb66f-48tzn             3/3       Running   0          1d  
kubectl logs cni-metrics-helper-6dcff5ddf4-v5l6d -n kube-system

cni-metrics-helper key log messages

# following shows the aggregated metrics. this can be optionally sent to cloudwatch
I0516 17:11:58.489648       7 metrics.go:350] Produce GAUGE metrics: ipamdActionInProgress, value: 0.000000
I0516 17:11:58.489657       7 metrics.go:350] Produce GAUGE metrics: assignIPAddresses, value: 2.000000
I0516 17:11:58.489665       7 metrics.go:350] Produce GAUGE metrics: totalIPAddresses, value: 11186.000000
I0516 17:11:58.489674       7 metrics.go:350] Produce GAUGE metrics: eniMaxAvailable, value: 800.000000
I0516 17:11:58.489685       7 metrics.go:340] Produce COUNTER metrics: ipamdErr, value: 1.000000
I0516 17:11:58.489695       7 metrics.go:350] Produce GAUGE metrics: eniAllocated, value: 799.000000
I0516 17:11:58.489715       7 metrics.go:350] Produce GAUGE metrics: maxIPAddresses, value: 11200.000000

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

Copy link
Contributor

@dbenhur dbenhur left a comment

Choose a reason for hiding this comment

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

Why is binary cni-metrics-helper/cni-metrics-helper committed to source repo?

@vpm-bradleyhession
Copy link

Bump

can we please get this merged? Especially since pulling the image in misc/cni_metrics_helper.yaml results in an error.

Thanks,

@xdrus
Copy link

xdrus commented Jun 19, 2018

Can we also have a clear description of IAM permissions needed to publish metrics to CW? Is having "cloudwatch:PutMetricData" enough?

Makefile Outdated Show resolved Hide resolved
cni-metrics-helper/cni-metrics-helper.go Show resolved Hide resolved
options.pullInterval, options.submitCW)

var kubeClient clientset.Interface
kubeClient, err = createKubeClient("", options.kubeconfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the apiserver parameter from the createKubeClient function since it is only used in one place and you are providing a constant value of an empty string.

cni-metrics-helper/cni-metrics-helper.go Show resolved Hide resolved
cni-metrics-helper/cni-metrics-helper.go Outdated Show resolved Hide resolved
pkg/k8sapi/discovery.go Show resolved Hide resolved
pkg/publisher/publisher.go Show resolved Hide resolved
}

// Get cloudwatch client
ec2MetadataClient := ec2metadatawrapper.New(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned on the declaration, the nil parameter here isn't clean.

@@ -0,0 +1,68 @@
// Code generated by MockGen. DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where this is used. I think it can be removed.

scripts/dockerfiles/Dockerfile.cni Show resolved Hide resolved
@liwenwu-amazon
Copy link
Contributor Author

@vpm-bradleyhession we are still in the process of code reviewing. At meantime, you can checkout the source code of this PR and build it.

@liwenwu-amazon
Copy link
Contributor Author

@xdrus cloudwatch:PutMetricData IAM should be enough to publish cloudwatch metrics. Please let me know if you see any issues thanks

@maxenglander
Copy link

Are there plans to move this forward or merge? My organization is exploring adopting EKS and we would like to have more insight and metrics into CNI.

@tabern tabern added this to the v1.5 milestone Mar 5, 2019
@mogren mogren mentioned this pull request Apr 25, 2019
@mogren
Copy link
Contributor

mogren commented Apr 25, 2019

Closing this PR in favor of #413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants