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

Basic Kubernetes Integration #441

Merged
merged 1 commit into from Oct 6, 2015
Merged

Basic Kubernetes Integration #441

merged 1 commit into from Oct 6, 2015

Conversation

paulbellamy
Copy link
Contributor

@tomwilkie, @peterbourgon any feedback would be great.

I'm not super-happy with the result, as there are still some shortcomings:

  • We can't yet correlate connections going through the kube-proxy into pseudo service nodes, which means that all connections go through the "Uncontained" pseudo-node. This is because I haven't been able to figure out how to do it.
  • We can't ever get full resolution on connections going through a service, as it might include a userspace hop (depending on configuration).
  • There is a bug in k8s where some pods are not listed in the API. I've raised an issue, and added a workaround.
  • Need to pass a --probe.kuberetes true flag on the master node to enable it (for now).
  • There may be some issues rendering incoming internet connections from external load-balancers, depending on how they do that routing, but none beyond what scope would already have.
  • Kubernetes uses a lot of ephemeral (high) ports, which breaks the assumption about a higher port number being the client, and causes the graph to be laid out strangely.

In the example, there are:

  • 3 php (frontend) pods, accepting incoming http requests, and making requests to redis
  • 1 redis-master pod, handling writes
  • 2 redis-slave pods (one per minion), handling reads. The redis-slave pods maintain an open socket to redis-master for replication.

Screenshots of it:
Pods view. I'm not sure if we should show the "2 containers" under each node, or if we should skip that.
screen shot 2015-09-29 at 16 45 40

Pods-by-Service view, where pods are grouped when they belong to a common service
screen shot 2015-09-29 at 16 45 53

As you can see, all the connections routing through "Uncontained" makes the output mostly useless. However, we can filter out the loads of kube-system pods, which clutter up the view considerably.

@@ -16,7 +16,6 @@ import (
"time"

docker "github.com/fsouza/go-dockerclient"

This comment was marked as abuse.

func (k *client) RemoveEventListener(c chan watch.Event) error {
k.lock.Lock()
defer k.lock.Unlock()
return k.removeEventListener(c)

This comment was marked as abuse.

@paulbellamy paulbellamy force-pushed the kubernetes branch 2 times, most recently from 7134805 to fe70172 Compare September 8, 2015 16:27
@paulbellamy paulbellamy force-pushed the kubernetes branch 5 times, most recently from caad1b2 to fd06706 Compare September 24, 2015 08:16
@paulbellamy paulbellamy force-pushed the kubernetes branch 10 times, most recently from 2917be9 to 914ed83 Compare October 1, 2015 08:28
@paulbellamy paulbellamy changed the title [WIP] Kubernetes Integration Basic Kubernetes Integration Oct 1, 2015
)
err := r.client.WalkServices(func(s Service) error {
nodeID := report.MakeServiceNodeID(s.Namespace(), s.Name())
result.Nodes[nodeID] = s.GetNode()

This comment was marked as abuse.

if s := strings.SplitN(id, "/", 2); len(s) == 2 {
result.LabelMajor = s[1]
result.Node.Metadata[kubernetes.Namespace] = s[0]
result.Node.Metadata[kubernetes.PodName] = s[1]

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor

Quick first pass; it looks pretty good. Shame we can't make the connections work properly; something to come back to perhaps.

@paulbellamy
Copy link
Contributor Author

@tomwilkie I suspect the connections is something obvious I'm missing, but yeah. Definitely to revisit.

@tomwilkie
Copy link
Contributor

Make sure we have a bug to track the connection improvements.

@@ -96,6 +97,9 @@ func makeReportPostHandler(a xfer.Adder) http.HandlerFunc {
return
}
a.Add(rpt)
if len(rpt.Pod.Nodes) > 0 {
addKubernetesTopologies()
}

This comment was marked as abuse.

@paulbellamy
Copy link
Contributor Author

I believe I've addressed all concerns with this, so if you could have another quick look, and let's merge this.

@tomwilkie
Copy link
Contributor

I haven't tested the code myself (involved setting up k8s). And I'm hesitant as I don't think anyone will be regularly running this, so I very much expect it to bit rot.

But given its not enabled by default, the risks of merging this are minimal. LGTM.

@tomwilkie
Copy link
Contributor

Pls squash!

@peterbourgon
Copy link
Contributor

I'll give it a review at EOD. Please wait until then :)

r.RLock()
defer r.RUnlock()
return r.items
}

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@peterbourgon
Copy link
Contributor

re: the racy thing, in the best case, you could return a deep copy, but I recognize that's super annoying... maybe just a warning comment on get and list methods to not modify the return values?

Other than those little nits, LGTM.

* Added helper for installing scope on gcloud
* Added topologies Pods and Pods-by-Service
* Uses k8s.io/kubernetes/pkg/client/cache for the client
* Filter kube-system nodes by default
* Only show the k8s topologies if we've received a non-empty k8s report
paulbellamy added a commit that referenced this pull request Oct 6, 2015
@paulbellamy paulbellamy merged commit 6ad182a into master Oct 6, 2015
@paulbellamy paulbellamy deleted the kubernetes branch October 6, 2015 13:49
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.

None yet

3 participants