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

Kubernetes UI graph component. #8716

Closed
wants to merge 1 commit into from
Closed

Conversation

jackgr
Copy link
Contributor

@jackgr jackgr commented May 22, 2015

This PR adds the graph component developed with the dashboard component and chrome (see PR #7056) to the Kubernetes UI. It is unchanged from the demo shown at the Galvanize event on 2/25/2015, but needs minor updates to reflect changes made to the UI since the dashboard and chrome were submitted.

The GraphTab uses a container called cluster-insight to retrieve information from the running cluster. See https://github.com/google/cluster-insight and https://registry.hub.docker.com/u/kubernetes/cluster-insight/.

If cluster-insight is not deployed, the Graph Tab still works, but shows no data. The user can select canned data to see how it works. When it is deployed in a cluster, the Graph Tab will find it and display its data.

@k8s-bot
Copy link

k8s-bot commented May 22, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@jackgr
Copy link
Contributor Author

jackgr commented May 22, 2015

Thanks to contributors @duftler, @unwornmoon, @jellzilla, @balavas, @EranGabber, @preillyme, @bcbroussard, @rrmckinley, @kenmoore, @kmerker.

cc @lavalamp

@jackgr jackgr mentioned this pull request May 22, 2015
12 tasks
@lavalamp
Copy link
Member

In the future, we need to develop things like this in the codebase a piece at a time...

@jackgr
Copy link
Contributor Author

jackgr commented May 22, 2015

@lavalamp Understood. Like the chrome and dashboard, the initial code base was built in 3 weeks for Galvanize. Sorry.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@lavalamp
Copy link
Member

Cluster insight thingy seems to require opening up docker's port; this is a giant security hole and so we are very deliberately using the unix socket to talk to docker. We won't be able to package it on a cluster like this, at least not by default.

@yllierop
Copy link
Contributor

@lavalamp what if cluster-insight was modified and talked to the unix socket and acted like a proxy for the other nodes running cluster-insight?

@abonas
Copy link
Contributor

abonas commented May 25, 2015

@preillyme Yes, developers can drop cluster-insight onto the master, as described in its installation notes, but the static UI needs it to be served from the cluster, so packaging it up as a service like the other addons makes sense.

@jackgr If the graph component is going to be merged and enabled by default as part of k8s, why the cluster-insight shouldn't be included and enabled by default in k8s setup as well?
Not including it means that the UI will not work out of the box, and every user will have to deploy something extra.

@jackgr
Copy link
Contributor Author

jackgr commented May 26, 2015

Note that the UI can be tested apart from cluster-insight, using the canned data, selectable from the UI or programatically.

@jackgr jackgr force-pushed the graph_tab branch 3 times, most recently from 2f89320 to 30d2ed0 Compare May 26, 2015 06:50
@lavalamp
Copy link
Member

@lavalamp what if cluster-insight was modified and talked to the unix socket and acted like a proxy for the other nodes running cluster-insight?

Then you would have to run it on every node, which is also not ideal.

@saad-ali
Copy link
Member

Thank you very much for the pull request - we sincerely appreciate the work that goes into sending us a change.

We are currently in a feature-freeze and general code slush as we work toward our 1.0 release. Our initial triage indicates that this is a non-critical change at this time, so we'd like to leave it open and revisit it after 1.0. We expect the freeze to wrap up in early to mid July.

If you feel that this triage is incorrect, please respond and let us know why. We're currently accepting changes that meet any of the following criteria:

  1. Changes directly related to v1.0 issues
  2. Changes addressing P0 bugs
  3. Changes to supported platforms to bring them up to par with the primary development platforms
  4. Docs, user experience, and test related fixes

@saad-ali saad-ali added this to the v1.0-post milestone May 26, 2015
@googlebot
Copy link

CLAs look good, thanks!

@jackgr
Copy link
Contributor Author

jackgr commented May 27, 2015

@saad-ali This PR is for work that was requested by the Kubernetes leads in February. It depended on #7056, which took a while to be merged and had follow on work in #7122. This PR was therefore delayed, but still expected. It would not be good if this PR didn't make the release. @lavalamp has been tracking this work from the outset, so please check with him regarding its status. Thanks.

@jackgr jackgr force-pushed the graph_tab branch 2 times, most recently from ea066d6 to db0c4b2 Compare May 27, 2015 02:54
@saad-ali
Copy link
Member

Thanks for the context Jack. Changing to 1.0 milestone.

@saad-ali saad-ali added this to the v1.0 milestone May 27, 2015
@jackgr jackgr force-pushed the graph_tab branch 11 times, most recently from d31362f to 13b4222 Compare June 25, 2015 03:52
@jackgr
Copy link
Contributor Author

jackgr commented Jun 29, 2015

Here's an update on this PR...

The cluster-insight data collector is now just an ordinary Kubernetes service. It no longer requires opening firewall ports on the minions, and installs by just submitting the YAML files for two RCs and a service. The README is updated to describe the new behavior.

Removing the WIP, since now that this blocker has been addressed, there are no known issues. It's ready to merge.

@jackgr jackgr changed the title WIP: Kubernetes UI graph component. Kubernetes UI graph component. Jun 29, 2015
@bgrant0607
Copy link
Member

add to whitelist

@k8s-bot
Copy link

k8s-bot commented Jun 29, 2015

GCE e2e build/test passed for commit b6b42fc.

@bgrant0607
Copy link
Member

Still has the issue that the graph tab will be nonfunctional for default cluster setups.

@bgrant0607 bgrant0607 self-assigned this Jun 29, 2015
@bgrant0607
Copy link
Member

This PR is enormous: 105 changed files with 174,475 additions and 3,690 deletions. Moreover, the whole GUI is enormous, requires a special build process and tools, and is in Javascript, which we can't adequately review.

As previously discussed IRL, the only viable path forward is to break the GUI back out into its own project, with its own source repo, continuous testing, and releases, and run it as a service on Kubernetes. The v1 API should be stable enough to make that feasible.

With respect to cluster-insight specifically, AIUI, it requires "root" in the cluster, since it directly accesses Docker. I haven't looked at it, I also doubt it would work for nodes running Rocket rather than Docker. We should look at the overlap with cadvisor and heapster so that we don't need to solve similar problems multiple times.

Additionally, at some point, we'll need to figure out how to make whole-system "monitoring" components such as this play nicely with namespace-based security policies (see, e.g., #7893).

cc @kmerker

@yllierop
Copy link
Contributor

I agree 100% with @bgrant0607 the direction that #10077 takes I think ultimately is the correct approach especially that the v1 API is now stable enough.

@lavalamp
Copy link
Member

cluster-insight running on every node is very unfortunate.

From glancing at the code it doesn't appear to actually proxy docker, which would be a security problem. But I'm not sure even about serving the read-only data. Needing root is another problem. Interoperability (with e.g. Rkt) is another. Do you really need that level of detail for the entire cluster? Does kubelet not report enough in the pod status?

@bgrant0607
Copy link
Member

Yes, we should improve PodStatus and/or cadvisor, if necessary.

Also note that @preillyme created https://github.com/kubernetes-ui/kube-ui

@yllierop
Copy link
Contributor

@bgrant0607 once we get #10077 working with the container built from https://github.com/kubernetes-ui/kube-ui and published at: gcr.io/kube-ui/kube-ui we can remove the www directory completely and close this PR. @jackgr can then submit the PR to the UI repo directly.

@k8s-bot
Copy link

k8s-bot commented Jun 30, 2015

GCE e2e build/test passed for commit b6b42fc.

@jackgr
Copy link
Contributor Author

jackgr commented Jun 30, 2015

We will move this code to https://github.com/kubernetes-ui/kube-ui, per the comment from @preillyme.

However, there are several issues raised in this thread that should be addressed for the record:

  • 79 of the 105 files are images, .gitkeep files or build output, which don't need code review.
  • 26 of the 105 files are source. These are the only files that need code review.
  • The build process is based on a boilerplate build file that reflects standard industry practice.
  • The cluster-insight container is a vanilla python application. It needs to be a member of the docker group to access the daemon, but does not need to run as root, and has no special requirements.
  • The container running on every node adds negligible CPU overhead, since the master caches the fetched data aggressively. Its memory footprint is 1/3 the size of fluentd.
  • When Rocket interoperability is needed, we will add it.

The most significant factor in the difficulty surrounding this UI may be the point raised earlier, noting that it's written in Javascript, which the Kubernetes team can't adequately review. Since Javascript is currently the predominant language for client side development, and seems unlikely to be replaced by Go any time soon, this is an issue that the Kubernetes team will probably need to come to terms with.

@jackgr jackgr closed this Jun 30, 2015
@stefwalter
Copy link
Contributor

Just for completeness, I can't help but comment:

The cluster-insight container is a vanilla python application. It needs to be a member of the docker group to access the daemon, but does not need to run as root, and has no special requirements.

The docker group (where available) is root equivalent. It is trivial to escalate to root via the docker daemon, and bypass auth, audit, and logging. Docker upstream states this themselves in their documentation. For this reason, most operating systems do not (or no longer) create the docker group by default.

@jackgr
Copy link
Contributor Author

jackgr commented Jun 30, 2015

It's worth noting that all of the current add ons and examples run as root. The plan is for that to change, but it hasn't yet, and it wasn't a blocker for getting the functionality integrated. Likewise, cluster-insight can be made to interact with Docker and/or Rocket through APIs given appropriate credentials, instead of using the Unix socket, but until we get to the point of locking everything down, it probably shouldn't be a blocker for getting the functionality integrated.

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