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

Deal with duplicated IP in the pod cache #252

Merged
merged 14 commits into from
Mar 4, 2020
Merged

Conversation

ltagliamonte-dd
Copy link
Contributor

@ltagliamonte-dd ltagliamonte-dd commented Jan 6, 2020

Trying to definitely fix issue #244 maintaining retro compatibility.

When the flag is set to true (default is false) in case the cache lookups returns more than 1 result,
a query to the API server is issued and a pod is returned only if it is running and not with host network.

I've also added some metrics in order to track the amount of requests that are performed to the api server.

@Jacobious52 or any other maintainers can you please check this out?

@coveralls
Copy link

coveralls commented Jan 6, 2020

Coverage Status

Coverage remained the same at 19.481% when pulling 4654208 on ltagliamonte-dd:master into 796b988 on jtblin:master.

k8s/k8s.go Outdated Show resolved Hide resolved
k8s/k8s.go Outdated Show resolved Hide resolved
metrics/metrics.go Outdated Show resolved Hide resolved
@ltagliamonte-dd
Copy link
Contributor Author

@Jacobious52 @jtblin any updates on this PR?

@ltagliamonte-dd
Copy link
Contributor Author

@Jacobious52 or @SharpEdgeMarshall do any of you have available cycles to look at this?
I'd really like to get a review and get this in for everyone.

@ltagliamonte-dd
Copy link
Contributor Author

Can someone please review this PR?
@struz @SharpEdgeMarshall @Jacobious52 @jtblin @jrnt30 ?

@SharpEdgeMarshall
Copy link
Collaborator

lgtm

@ltagliamonte-dd
Copy link
Contributor Author

Can someone please review and merge this PR?
@struz @Jacobious52 @jtblin @jrnt30 ?

Copy link

@sean- sean- left a comment

Choose a reason for hiding this comment

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

A bunch of nit-picky things that I think will help future readers of this code. My one big question is, is there value in conditionalizing this at all? Why not unconditionally support resolving a pod when there are multiple pods with the same IP?

README.md Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
Namespace: namespace,
Subsystem: "iam",
Name: "k8s_dup_req_count",
Help: "Total number of K8s Api requests performed when duplicated pods are identified in the cache.",
Copy link

Choose a reason for hiding this comment

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

s/Api/API/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be capitalised to match the official Kubernetes docs: https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/

Suggested change
Help: "Total number of K8s Api requests performed when duplicated pods are identified in the cache.",
Help: "Total number of K8s API requests performed when duplicated pods are identified in the cache.",

k8s/k8s.go Outdated Show resolved Hide resolved
k8s/k8s.go Outdated Show resolved Hide resolved
metrics/metrics.go Show resolved Hide resolved
metrics/metrics.go Show resolved Hide resolved
metrics/metrics.go Show resolved Hide resolved
metrics/metrics.go Show resolved Hide resolved
metrics/metrics.go Show resolved Hide resolved
@ltagliamonte-dd
Copy link
Contributor Author

I'm happy to work on any change that can help this to get merged.
Thanks.
@Jacobious52 @jtblin

Copy link
Collaborator

@mwhittington21 mwhittington21 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me except the argument naming. I've provided the suggestions for everything that needs to change, if you agree just accept all those changes and squash the commits and I'll get this merged and into a release ASAP.

README.md Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
k8s/k8s.go Outdated Show resolved Hide resolved
k8s/k8s.go Outdated Show resolved Hide resolved
k8s/k8s.go Outdated Show resolved Hide resolved
Namespace: namespace,
Subsystem: "iam",
Name: "k8s_dup_req_success_count",
Help: "Total number of times we successfully retrieve the pod from the K8s Api.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Help: "Total number of times we successfully retrieve the pod from the K8s Api.",
Help: "Total number of times we successfully retrieve the pod from the K8s API.",

server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
@ltagliamonte-dd
Copy link
Contributor Author

@mwhittington21 thank you for the review I've addressed all you suggestions via 4654208

Looking forward to see this merged and released.
Thank you for you time!

You can squash all commits on merge using:
select-squash-and-merge-from-drop-down-menu

@struz struz merged commit 42d4453 into jtblin:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants